Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add initial structure of the project and first feature to validate asyncapi files #5

Merged
merged 44 commits into from
Jun 18, 2021

Conversation

jotamusik
Copy link
Contributor

@jotamusik jotamusik commented Apr 13, 2021

Description

  • Use Typescript
  • Use Jest as test runner
  • Generate a first simple cli to understand how ink and meow works
  • Develop a first feature to see the project's skeleton

Related issue(s)

package.json Outdated Show resolved Hide resolved
@derberg
Copy link
Member

derberg commented Apr 14, 2021

Looks good so far, I would not worry much about project structure as this can always be improved later, most important is to have first hook, first simple functionality like validation and then the rest will be easier and others will know how to contribute further

@jotamusik jotamusik changed the title Initial Structure + First Approach feat: Initial Structure + First Approach Apr 14, 2021
@jotamusik jotamusik changed the title feat: Initial Structure + First Approach feat: initial structure + first approach Apr 14, 2021
@jotamusik
Copy link
Contributor Author

Looks good so far, I would not worry much about project structure as this can always be improved later, most important is to have first hook, first simple functionality like validation and then the rest will be easier and others will know how to contribute further

Working on it!! 😛

@Souvikns
Copy link
Member

I was thinking of an approach if we were going to use meow for parsing CLI commands

#!/usr/bin/env node
import React from 'react';
import {render} from 'ink';
import meow from 'meow';
import App from './ui';
import Generate from './generate';

const cli = meow(`
	Usage
	  $ generate <asyncapi> <template>

	Options
		--name  Your name

	Examples
	  $ learnINK --name=Jane
	  Hello, Jane
`, {
	flags: {
		name: {
			type: 'string'
		}
	}
});

let [command, ...args] = cli.input;

switch(command) {
	case "generate":
		render(<Generate file={cli.input[1]} />);
}

We can make different ink components for every command and run them like this.
or we could also create a single UI component where we can check for all the different commands and render different UI accordingly.

@jotamusik according to you what should we go with.

@jotamusik
Copy link
Contributor Author

yeah @Souvikns!! that's the idea, this is one of the steps that will drive to the approach you mention 😋

Thanks for the feedback!! 🤗

@derberg
Copy link
Member

derberg commented Apr 16, 2021

Screenshot 2021-04-16 at 13 55 38

so what is missing, when do you feel comfortable to get some code review. I guess that just some cleanup is needed and we can also slowly start splitting the initial issue into separate issues for separate detaild discussion (if needed).

I want asyncapi context -f <filepath> so much that you can't imagine 😄

package.json Outdated Show resolved Hide resolved
@Souvikns
Copy link
Member

Souvikns commented Apr 16, 2021

Screenshot 2021-04-16 at 13 55 38

so what is missing, when do you feel comfortable to get some code review. I guess that just some cleanup is needed and we can also slowly start splitting the initial issue into separate issues for separate detaild discussion (if needed).

I want asyncapi context -f <filepath> so much that you can't imagine 😄

we can have a separate command namely context and have operations to add, remove or change the filename of the spec file. then if the context file is not set then we can throw an error or ask to set the context.

also, I think separate issues for all different commands will help.

@Souvikns
Copy link
Member

@jotamusik I have a doubt about the argument parser meow
I was playing around with commander and meow in general, I don't have much experience with meow. Could we achieve this with meow through any plugin or do we need to write specific code for this.

commander

when I use asyncapi --help

Usage: asyncapi [options] [command]

Options:
  -V, --version                             output the version number
  -h, --help                                display help for command

Commands:
  generate [options] <asyncapi> <template>

and when I use asyncapi generate --help

Usage: asyncapi generate [options] <asyncapi> <template>

Options:
  -i, --install  installs the template and its dependencies (defaults to false)
  -h, --help     display help for command

It generates help option for all the commands , this helps users as different commands have different options that it supports.

meow

when I use asyncapi --help

  Usage
  $ generate <asyncapi> <template>

  Options
  -V, --version    output the version number
  -h, --help       display help for command

and when I use asyncapi generate --help the auto help command does not work but it returns this object

{
  input: [ 'generate' ],
  flags: { help: true },
  unnormalizedFlags: { help: true },
  pkg: {
    name: 'mew',
    version: '1.0.0',
    description: '',
    main: 'index.js',
    scripts: { test: 'echo "Error: no test specified" && exit 1' },
    keywords: [],
    author: '',
    license: 'ISC',
    dependencies: { 'cli-meow-help': '^2.0.2', commander: '^7.2.0', meow: '^9.0.0' },
    readme: 'ERROR: No README data found!',
    _id: 'mew@1.0.0'
  },
  help: '\n' +
    '  Usage\n' +
    '  $ generate [options] <asyncapi> <template>\n' +
    '\n' +
    '  Options\n' +
    '  -V, --version    output the version number\n' +
    '  -h, --help       display help for command\n',
  showHelp: [Function: showHelp],
  showVersion: [Function: showVersion]
}

we can create our own custom help string for each commands though, but is there a way of doing this automatically in meow.

@aayushmau5
Copy link
Member

@jotamusik I have a doubt about the argument parser meow
I was playing around with commander and meow in general, I don't have much experience with meow. Could we achieve this with meow through any plugin or do we need to write specific code for this.

commander

when I use asyncapi --help
....

As far as I know, we can't do this with meow. I maybe wrong though.

But this makes me wonder, should we use the commander approach by doing cli <some-action> <options> (ex. cli generate) or just use flags/options for everything(like cli --generate)?

What are your thoughts? @jotamusik @derberg @Souvikns

@derberg
Copy link
Member

derberg commented Apr 18, 2021

Looks like meow want's to support autogeneration of help -> sindresorhus/meow#80

no strong opinion for commander or yargs instead of meow. Gatsby CLI uses for example ink with yargs 🤷🏼

@aayushmau5 have a look at #1 to see how we want to design commands

@Souvikns
Copy link
Member

Souvikns commented Apr 18, 2021

I was playing with meow a little bit and I think we can create our own help functions and make it just like commander from checking the command and the help flag

const meow = require('meow');
const mewHelp = require('cli-meow-help');

const cli = meow(`
Usage
$ generate [options] <asyncapi> <template>

Options
-V, --version    output the version number
-h, --help       display help for command
`);

const commands = {
    "generate": `
Usage
asyncapi generate [options] <asyncapi> <template>

Options
-i, --install   installs the template and its dependencies 
-h, --help      display help for command
`
}
const helperText = (command, flag) => {
    if(commands[command] && flag){
        console.log(commands[command])
    }
}

helperText(cli.input[0], cli.flags.help)

Also found this library https://www.npmjs.com/package/cli-meow-help, it uses chalk to create a good-looking helper text for commander but it has a problem that we can't turn of any sections like even if we don't have any commands it will still render the command section and leave it empty.

@derberg
Copy link
Member

derberg commented May 5, 2021

@jotamusik hey, where are we with this one. Do you need some help? Just want to make sure you are not trying to be too perfect with first PR. Keep in mind we can improve further and once we get this one merged we will also enable more people to help and improve further.

@jotamusik
Copy link
Contributor Author

Hi! @derberg I'm working on a first minimal feature of validation, so still WIP without trying to be perfect 😅

I know it's more time than expected, but it's because I have been busy the past weeks

@jotamusik
Copy link
Contributor Author

I'm having some issues with the tests of the validation component and I think that it's related with the useEffect() and the component lifecycle.

If anyone can take a look, it would be great! 😛

@Souvikns
Copy link
Member

The useEffect is not working when the component is called in the test environment. There is some problem with the ink-testing-library I found this vadimdemedes/ink-testing-library#3

@jotamusik jotamusik requested a review from derberg June 11, 2021 11:31
Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jotamusik I left a follow up comments in previous comments that were not answered

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
src/CliModels.ts Outdated Show resolved Hide resolved
src/cli.ts Outdated Show resolved Hide resolved
src/cli.ts Show resolved Hide resolved
src/hooks/validation/models.ts Outdated Show resolved Hide resolved
src/hooks/validation/models.ts Show resolved Hide resolved
src/hooks/validation/models.ts Outdated Show resolved Hide resolved
@jotamusik
Copy link
Contributor Author

I think that we are done for this first version!! 🤔

Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

few minors but need to be fixed before the merge

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/hooks/validation/models.ts Outdated Show resolved Hide resolved
src/hooks/validation/models.ts Outdated Show resolved Hide resolved
@derberg derberg changed the title feat: initial structure + first approach feat: add initial structure of the project and first feature to validate asyncapi files Jun 18, 2021
Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀

@jotamusik Congrats!!! Now let us see if we did not forget about anything for release automation 🤞🏼 hold my 🍺

@derberg derberg merged commit 8dbc2c7 into asyncapi:master Jun 18, 2021
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 0.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

peter-rr added a commit to peter-rr/cli that referenced this pull request Dec 13, 2023
…asyncapi#5)

* Add new method to unify logic so metrics are collected when any command is invoked

* Remove some unnecessary code

* Change NR license key

* Fix 'init()' to return a proper value for the command invoked

* Unify logic for the remaining commands

* Unify logic also for 'action.executed' metric

* Rollback last changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants